Compiler: add locations to all expanded macro arguments #7008

makenowjust · 2018-10-30T06:52:08Z

This PR changes compiler to add locations to all expanded macro arguments if possible. It makes helpful error message on macro expansion.

For example the following code makes an error with message undefined constant BadType.

class Foo
  getter foo : BadType
end

However, currently the compiler shows expanded macro code and reports expanded macro line as error location. It is confusing for Crystal beginners I think.

$ crystal run foo.cr
Error in foo.cr:2: expanding macro

  getter foo : BadType
  ^~~~~~

in foo.cr:2: expanding macro

  getter foo : BadType
  ^

in macro 'getter' expanded macro: macro_4384842512:113, line 4:

   1.
   2.
   3.
>  4.             @foo : BadType
   5.
   6.             def foo : BadType
   7.               @foo
   8.             end
   9.
  10.
  11.
  12.

undefined constant BadType

After this PR, the compiler shows such a simple error:

$ bin/crystal foo.cr
Error in foo.cr:2: expanding macro

  getter foo : BadType
  ^~~~~~

in foo.cr:2: undefined constant BadType

  getter foo : BadType
               ^~~~~~~

Great!

Technical Note

I believe this PR is awesome and useful, however, some developers (especially compiler specialist @asterite) think it is unacceptable because emitting location pragmas more makes breaking change and they know it is unuseful in some cases. (ref. #3858)

Don't worry! This PR does not add emitting location pragmas actually. Instead of emitting location pragmas to expanded code, it saves a pair of expanded code position and location pragmas of its position on macro expansion (it is called as invisible_loc_pragmas in source code.), and the lexer processes this data on parsing expanded code. It is cleaner and better than embedding location pragmas.
(However we cannot remove location pragmas from language because it has use-case still, e.g. ECR)

@bew This PR solves #6125 indirectly. What do you think?

Thank you 😄

straight-shoota · 2018-10-30T07:46:38Z

For your example, this is great because the failing BadType is both in the original source as well as the expanded macro.

But what if the error was introduced only in the macro, with no direct equivalent in the original code? Let's just use the same code and error for example, but a custom macro for defining it:

macro define_foo
  @foo : Bad type
  def foo : Bad type
    @foo 
  end
end

class Foo
  define_foo
end

What error would this produce?

makenowjust · 2018-10-30T07:53:09Z

@straight-shoota no Bad type, just BadType?

Error in foo.cr:9: expanding macro

  define_foo
  ^~~~~~~~~~

in foo.cr:9: expanding macro

  define_foo
  ^

in macro 'define_foo' foo.cr:1, line 1:

>  1.   @foo : BadType
   2.   def foo : BadType
   3.     @foo
   4.   end

undefined constant BadType

It is same as current.

bew · 2018-10-30T08:28:56Z

@makenowjust I read the title of the PR, I knew exactly what was going on, as I've been thinking about doing exactly that (but by naively using loc pragma^^)

I'm glad to see that you took the time and did it :D
I'll read that later

bew · 2018-10-30T08:38:07Z

src/compiler/crystal/macros/interpreter.cr

@@ -512,33 +517,33 @@ module Crystal
    end

    def visit(node : TupleLiteral)
-      @last = TupleLiteral.map(node.elements) { |element| accept element }.at(node)
+      @last = TupleLiteral.map(node.elements) { |element| accept element }


Why remove the location of these new nodes? (here and below)

For preventing to report macro location as error location. ~~However, I forgot they are used to report macro interpreter error, so this is wrong.~~

EDIT(2018-10-31): No. There is nowhere to use these locations.

For preventing to report macro location as error location

sry but I still don't get it, could you give an example?

@bew You have this code:

macro foo {% x = 1 %} {{ x }} + "foo" end foo

And you will get such a error with bad location when evaluated macro node has its location:

$ bin/crystal run foo.cr Error in foo.cr:6: expanding macro foo ^~~ in foo.cr:2: no overload matches 'Int32#+' with type String Overloads are: - Int32#+(other : Int8) - Int32#+(other : Int16) - Int32#+(other : Int32) - Int32#+(other : Int64) - Int32#+(other : Int128) - Int32#+(other : UInt8) - Int32#+(other : UInt16) - Int32#+(other : UInt32) - Int32#+(other : UInt64) - Int32#+(other : UInt128) - Int32#+(other : Float32) - Int32#+(other : Float64) - Number#+() {% x = 1 %} ^

I think now it is wrong that evaluated macro node has location.

@asterite You added locations to evaluated macro node in #5597, and you said:

In theory all nodes should have a location.

However, what is theory? I'm wondering when these locations are used and useful.

I think it is creepy that evaluated macro node has location because it is just a value except passed arguments. And I think these locations are never used.

asterite · 2018-10-30T14:11:36Z

Hey @makenowjust ! I think this is a great solution. I found it hard to understand at first, but it makes total sense. I solves very well one of the problems of text macros, basically that you loose the reference to the original node locations. I guess the performance impact is low because pragmas will only be checked in the lexer when lexing a macro expansion.

I'm not sure about the invisible term. Maybe we could call them "macro expansion pragmas" to make it clear that these result from macro expansion.

asterite · 2018-10-30T14:12:51Z

src/compiler/crystal/syntax/lexer.cr

+
+    alias LocPragma = LocStackPragma | LocSetPragma
+
+    enum LocStackPragma


What do you think if we separate this into two types? It can be LocPushPragma and LocPopPragma. Maybe the code will be even smaller. And the case bellow will be "complete" (right now there's a hidden else)

bew · 2018-10-30T14:55:17Z

@makenowjust will the example at #6125 (comment) work with this PR? Or does it need sth else?

makenowjust · 2018-10-31T04:14:55Z

@bew No, unfortunately... However I think it is another problem from this PR. The reason what #6125 (comment) does not work is for re-raising an error with node calling macro location here:

  def eval_macro(node)
    yield
  rescue ex : MacroRaiseException
    node.raise ex.message, exception_type: MacroRaiseException
  rescue ex : Crystal::Exception
    node.raise "expanding macro", ex
  end

I don't know why this is needed. Sorry.

@asterite

Thank you @asterite crystal-lang#7008 (comment) crystal-lang#7008 (comment) Split `LocStackPragma` into `LocPushPragma` and `LocPopPragma` Rename `invisible_loc_pragmas` to `macro_expansion_pragmas`

@asterite

Thank you @asterite crystal-lang#7008 (comment) crystal-lang#7008 (comment) Split `LocStackPragma` into `LocPushPragma` and `LocPopPragma` Rename `invisible_loc_pragmas` to `macro_expansion_pragmas`

@asterite

Thank you @asterite crystal-lang#7008 (comment) crystal-lang#7008 (comment) Split `LocStackPragma` into `LocPushPragma` and `LocPopPragma` Rename `invisible_loc_pragmas` to `macro_expansion_pragmas`

makenowjust · 2018-11-13T16:50:59Z

ping. Who is interested in this still?

sdogruyol

Thank you @makenowjust 👍

bew reviewed Oct 30, 2018

View reviewed changes

RX14 requested a review from asterite October 30, 2018 12:41

asterite reviewed Oct 30, 2018

View reviewed changes

makenowjust force-pushed the feature/add-location-to-all-expanded-macro-args branch from baa755e to f27e704 Compare November 3, 2018 03:56

makenowjust added 2 commits November 3, 2018 23:49

Add location to all expanded macro arguments

a10b27f

Refactor implementation

313c40e

Thank you @asterite crystal-lang#7008 (comment) crystal-lang#7008 (comment) Split `LocStackPragma` into `LocPushPragma` and `LocPopPragma` Rename `invisible_loc_pragmas` to `macro_expansion_pragmas`

makenowjust force-pushed the feature/add-location-to-all-expanded-macro-args branch from f27e704 to 313c40e Compare November 3, 2018 14:51

Invalidate name size of TypeException caused by some macro expansion

d71ae1b

RX14 approved these changes Nov 21, 2018

View reviewed changes

RX14 requested review from asterite and bcardiff November 21, 2018 21:56

asterite approved these changes Nov 21, 2018

View reviewed changes

sdogruyol approved these changes Dec 3, 2018

View reviewed changes

RX14 merged commit 9c42d31 into crystal-lang:master Dec 4, 2018

RX14 added this to the 0.27.1 milestone Dec 4, 2018

RX14 added kind:feature topic:compiler labels Dec 4, 2018

bew mentioned this pull request Dec 5, 2018

Macro raise doesn't keep location #7147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compiler: add locations to all expanded macro arguments #7008

Compiler: add locations to all expanded macro arguments #7008

makenowjust commented Oct 30, 2018

straight-shoota commented Oct 30, 2018

makenowjust commented Oct 30, 2018

bew commented Oct 30, 2018

bew Oct 30, 2018

makenowjust Oct 30, 2018 •

edited

Loading

bew Oct 30, 2018

makenowjust Oct 31, 2018

asterite commented Oct 30, 2018

asterite Oct 30, 2018

bew commented Oct 30, 2018

makenowjust commented Oct 31, 2018

makenowjust commented Nov 13, 2018 •

edited

Loading

sdogruyol left a comment


		alias LocPragma = LocStackPragma \| LocSetPragma

		enum LocStackPragma

Compiler: add locations to all expanded macro arguments #7008

Compiler: add locations to all expanded macro arguments #7008

Conversation

makenowjust commented Oct 30, 2018

Technical Note

straight-shoota commented Oct 30, 2018

makenowjust commented Oct 30, 2018

bew commented Oct 30, 2018

bew Oct 30, 2018

Choose a reason for hiding this comment

makenowjust Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

bew Oct 30, 2018

Choose a reason for hiding this comment

makenowjust Oct 31, 2018

Choose a reason for hiding this comment

asterite commented Oct 30, 2018

asterite Oct 30, 2018

Choose a reason for hiding this comment

bew commented Oct 30, 2018

makenowjust commented Oct 31, 2018

makenowjust commented Nov 13, 2018 • edited Loading

sdogruyol left a comment

Choose a reason for hiding this comment

makenowjust Oct 30, 2018 •

edited

Loading

makenowjust commented Nov 13, 2018 •

edited

Loading